Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

overhaul to allow for different labels #8

Merged
merged 9 commits into from
Feb 12, 2020
Merged

overhaul to allow for different labels #8

merged 9 commits into from
Feb 12, 2020

Conversation

simeonschaub
Copy link
Owner

@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

Merging #8 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #8   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          26     29    +3     
=====================================
+ Hits           26     29    +3
Impacted Files Coverage Δ
src/OptionalArgChecks.jl 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aecc643...4356233. Read the comment docs.

end

@dynamo function skipargcheck(x...)
struct ElideCheck{label}
ElideCheck(label::Symbol) = new{label}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason, why label is not a field, but a type parameter?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to do some benchmarks, whether there is actually any difference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding. You are talking about compile time performance, not run time right? Because as I understand it at run time no ElideCheck objects exist anymore.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, now that I've looked into this, a type parameter is definitely the way to do this. A @dynamo cannot access any fields of ElideCheck, so the only way to access the label would be to insert a statement into the IR, which wouldn't even work in this case. This makes sense, if you think about it, because otherwise IRTools would have to recompile f every time, since label is not a compile time constant.

@jw3126
Copy link
Contributor

jw3126 commented Feb 12, 2020

julia> using ArgCheck, OptionalArgChecks
[ Info: Precompiling OptionalArgChecks [dfbeeb84-381a-44da-9ec9-a723abf299c7]

julia> f(x) = @argcheck x > 0
f (generic function with 1 method)

julia> @skipargcheck f(-1)

julia> 

👍

@jw3126
Copy link
Contributor

jw3126 commented Feb 12, 2020

I registered an ArgCheck version that has markers:

JuliaRegistries/General#9353 (comment)

@simeonschaub simeonschaub merged commit 49be087 into master Feb 12, 2020
@simeonschaub simeonschaub mentioned this pull request Feb 12, 2020
@jw3126
Copy link
Contributor

jw3126 commented Feb 12, 2020

Great! @simeonschaub what do you think about tagging a release?

@simeonschaub
Copy link
Owner Author

Already done: JuliaRegistries/General#9359. Still waiting for 0.1 to be registered though:grin:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants